Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalizing OG errors #1600

Merged
merged 5 commits into from
Oct 20, 2023
Merged

Normalizing OG errors #1600

merged 5 commits into from
Oct 20, 2023

Conversation

otherview
Copy link
Contributor

@otherview otherview commented Oct 13, 2023

Why this change is needed

Part of https://github.com/obscuronet/obscuro-internal/issues/2275

  • Normalizes the error return structure
  • Adds a test to check error returns
  • Cleans up the error handling that was repeated in a few different places
  • Uses the same JSON Response format as geth

The ethereum error structure is : {"jsonrpc":"2.0","id":1,"error":{"code":-32602,"message":"invalid argument 0: json: cannot unmarshal hex string of odd length into Go value of type common.Address"}}

The gateway error structure is: {"error":{"message":"unable to create VK encryptor - unable to create vk encryption for request - invalid viewing key signature for requested address"}}

Changing the error struct to match the jrpc error structure

What changes were made as part of this PR

Please provide a high level list of the changes made

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

  • PR checks reviewed and performed

@coderabbitai
Copy link

coderabbitai bot commented Oct 13, 2023

Walkthrough

The changes primarily focus on improving error handling, logging, and response writing across the codebase. New functions have been introduced to handle Ethereum-related and general errors. Unused imports and constants have been removed, and test cases have been updated. The changes also include modifications to handle disconnects and nil responses more effectively.

Changes

File Summary
.../obscurogateway/obscurogateway_test.go Added new tests and modified existing ones. Removed return value from createObscuroNetwork function.
.../accountmanager/account_manager.go Improved handling of user disconnects and error handling in executeSubscribe function.
.../api/routes.go Updated error handling, logging, and response writing in request handler functions.
.../api/utils.go Added new functions handleEthError and handleError for improved error handling.
.../common/constants.go Removed unused import and constant.
.../common/json.go Introduced new package common with JSONRPCMessage and JSONError structs for handling JSON-RPC messages.
.../userconn/user_conn.go Removed unused imports and constants. Improved error handling in UserConn interface and its implementations.
.../wallet_extension.go Improved error and response handling in ProxyEthRequest function.
.../test/wallet_extension_test.go Removed unused import and updated a test case.

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.
  • Additionally, enter @coderabbitai pause to halt the bot's ongoing reviews as any issue or review comment. To
    resume reviews, enter @coderabbitai resume as any issue or review comment.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Commits Files that changed from the base of the PR and between 3d3c12c and 3085d91.
Files selected for processing (8)
  • integration/obscurogateway/obscurogateway_test.go (6 hunks)
  • tools/walletextension/accountmanager/account_manager.go (1 hunks)
  • tools/walletextension/api/routes.go (5 hunks)
  • tools/walletextension/api/utils.go (2 hunks)
  • tools/walletextension/common/constants.go (2 hunks)
  • tools/walletextension/common/json.go (1 hunks)
  • tools/walletextension/userconn/user_conn.go (6 hunks)
  • tools/walletextension/wallet_extension.go (1 hunks)
Files skipped from review due to trivial changes (4)
  • tools/walletextension/api/routes.go
  • tools/walletextension/common/constants.go
  • tools/walletextension/userconn/user_conn.go
  • tools/walletextension/wallet_extension.go
Additional comments (Suppressed): 12
tools/walletextension/common/json.go (1)
  • 1-20: The new JSONRPCMessage and JSONError types are well-structured and align with the JSON-RPC specification. The use of json.RawMessage for ID, Params, and Result allows for flexibility in handling different data types. The JSONError type also aligns with the JSON-RPC error object specification, with Code, Message, and optional Data fields. This should improve error handling consistency across the codebase.
tools/walletextension/accountmanager/account_manager.go (1)
  • 312-320: The error handling mechanism has been updated to directly write the error message to the user connection instead of using the HandleError method. This change aligns with the PR's goal of normalizing error handling. However, the comment on line 315 indicates that there is still work to be done to properly handle disconnections on the user side. This should be addressed to ensure robust error handling.
- userConn.HandleError(err.Error())
+ err = userConn.WriteResponse([]byte(err.Error()))
+ if err != nil {
+     m.logger.Error("unable to close connection with the user", log.ErrKey, err)
+ }
tools/walletextension/api/utils.go (3)
  • 83-85: The function getUserID has been left unchanged. It retrieves the user ID from either query parameters or the URL path.

  • 87-119: The new function handleEthError is introduced to handle Ethereum related errors. It creates a JSON-RPC error message and writes it back to the client. The function takes in an RPCRequest, a UserConn, a logger, and an error. It constructs a JSONRPCMessage with the error details and sends it back to the client. The error handling and logging are done correctly.

  • 121-125: The function handleError is introduced to handle general errors. It writes the error message back to the client. The function takes in a UserConn, a logger, and an error. The error handling and logging are done correctly.

integration/obscurogateway/obscurogateway_test.go (7)
  • 3-24: The import statements have been updated to include new packages for error handling and JSON-RPC messages. Ensure that these packages are correctly installed and available in the project environment.

  • 50-55: The createObscuroNetwork function no longer returns a value. Ensure that this change does not affect other parts of the code that might be expecting a return value from this function.

  • 81-97: Two new test functions, testAreTxsMinted and testErrorHandling, have been added to the test suite. Ensure that these tests are correctly implemented and that they cover all the necessary scenarios.

  • 99-110: The wallet creation process has been updated to use wallet.NewInMemoryWalletFromConfig instead of wallets.L2FaucetWallet. Ensure that this change does not affect the functionality of the tests.

  • 122-125: The transaction receipt status is now being checked directly in the test function. This is a good practice as it allows for immediate feedback if the transaction fails.

  • 127-168: The testErrorHandling function has been added to test the new error handling mechanism. It makes several requests with different error scenarios and checks the response format. This is a good practice as it ensures that the error handling mechanism works as expected.

  • 222-228: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [222-245]

The createObscuroNetwork function no longer returns a value. This change simplifies the function and makes it easier to understand. However, ensure that this change does not affect other parts of the code that might be expecting a return value from this function.

tools/walletextension/api/utils.go Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between 3085d91 and 288b591.
Files selected for processing (1)
  • tools/walletextension/api/utils.go (2 hunks)
Additional comments (Suppressed): 3
tools/walletextension/api/utils.go (3)
  • 3-13: The new hunk introduces two new imports: github.com/obscuronet/go-obscuro/go/common/log and github.com/ethereum/go-ethereum/log. Ensure that these packages are used appropriately in the code and that they do not conflict with each other.

  • 88-120: The handleEthError function is introduced to handle Ethereum-related errors. It creates a JSONRPCMessage with the error details and writes it back to the user. The function handles the case where the request is nil and sets default values for method and id. It also handles any errors that occur during marshalling the JSONRPCMessage and writing the response back to the user. This is a good practice as it ensures that the user is always informed about what went wrong.

  • 122-126: The handleError function is introduced to handle generic errors. It writes the error message back to the user and handles any errors that occur during this process. This is a good practice as it ensures that the user is always informed about what went wrong.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 2

Commits Files that changed from the base of the PR and between 288b591 and 6fc2c4d.
Files selected for processing (3)
  • tools/walletextension/api/utils.go (2 hunks)
  • tools/walletextension/test/wallet_extension_test.go (2 hunks)
  • tools/walletextension/wallet_extension.go (2 hunks)
Files skipped from review due to trivial changes (2)
  • tools/walletextension/test/wallet_extension_test.go
  • tools/walletextension/wallet_extension.go

tools/walletextension/api/utils.go Show resolved Hide resolved
tools/walletextension/api/utils.go Show resolved Hide resolved
Copy link
Contributor

@zkokelj zkokelj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@otherview otherview merged commit ebcc4aa into main Oct 20, 2023
2 checks passed
@otherview otherview deleted the pedro/gw_errors branch October 20, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants